fix: Throw MQTT authentication errors as authentication related exceptions #634
Conversation
…tions The intent is to allow callers to catch this to know when they need to re-authenticate a device and obtain new mqtt params.
allenporter
force-pushed
the
authentication-error-startup
branch
from
4e32aeb to
2d04e37
Compare
There was a problem hiding this comment.
Pull request overview
This PR improves MQTT exception handling by introducing specific exception types for authorization errors, allowing callers to differentiate authentication failures from general MQTT connection issues and take appropriate action such as re-authenticating devices.
- Introduces
RoborockInvalidCredentialsexception for MQTT authorization errors (reason code 135) - Adds comprehensive test coverage for different MQTT error scenarios
- Updates documentation to clarify exception handling behavior
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/mqtt/test_roborock_session.py | Adds parameterized tests covering authorization errors (rc=135), other MQTT code errors, general MQTT errors, and unexpected exceptions |
| roborock/mqtt/session.py | Updates MqttSessionException docstring to clarify that specific error conditions may raise other RoborockException subtypes |
| roborock/mqtt/roborock_session.py | Implements authorization error detection using reason code 135 and raises RoborockInvalidCredentials for auth failures, with fallback to MqttSessionException for other errors |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
this is good! Only note: Unathorized does not only mean they need new credentials, it could be that the specific userdata is rate limited for <=24 hours.
Getting new credentials will fix the problem, waiting will also fix the problem (if it is a rate limit). Just important we make that clear.
|
I've updated the exception to be a different exception given the ambiguity in the scenarios. I think forcing reauth behavior may be a mistake so for now we'll keep them separate. |
2 participants
Update the MQTT exception handling to carve out a different exception type for authorization errors. Today callers do not have the ability to handle special authentication errors like this:
Now this will be thrown with
RoborockInvalidCredentials. Callers can catch this to know when they need to re-authenticate a device and obtain new mqtt params.